✅ Deploy Preview for stacks ready!

Name	Link
🔨 Latest commit	946a968
🔍 Latest deploy log	https://app.netlify.com/sites/stacks/deploys/635c76165e9d1f0009abb405
😎 Deploy Preview	https://deploy-preview-1144--stacks.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

The current method of triggering a sort in tables is not accessible. This is due to the fact that you are attaching an event listener to the click event on an element that naturally is not clickable; therefore, keyboard only users are not capable of interacting with the headers since focus is never moved onto them.

My proposed syntax moves the contents of a table header cell into a button, which is where the data-action attribute will now attentatively live.

<th data-s-table-target="column" class="s-table--sortable-column">
    <button class="s-table--clickable-header" data-action="click->s-table#sort">
        Season
        @Svg.ArrowUpSm.With("js-sorting-indicator js-sorting-indicator-asc d-none")
        @Svg.ArrowDownSm.With("js-sorting-indicator js-sorting-indicator-desc d-none")
        @Svg.ArrowUpDownSm.With("js-sorting-indicator js-sorting-indicator-none")
    </button>
</th>

This PR draft adds two new CSS classes for table header cells: .s-table--sortable-column and .s-table--clickable-header. I will leave it to the Stacks team to decide the best way to organize this Less and naming of the classes. But effectively, new Less needs to be introduced that does the following:

* Clear a button's appearance so that it looks no different from a regular table header cell

* Remove padding from `<th>` and move that to the `<button>` but only when a `<button>` exists

I like it! The only issue I see is with the naming of .s-table--clickable-header. I'd like to keep class names relating to presentation and not tied to their action/events (look away from .s-expandable 😝). That way, if the structure needs to change or styling works in context outside of a given action that can be attached, the class name will still make sense. I'll think on a more appropriate name, but feel free to make suggestions or tell me I'm being foolish. I haven't looked at the diff yet, so it's completely possible I'm speaking nonsense.

Now, in terms of discussion: I have introduced a ensureHeadersAreClickable method that transforms the old table syntax to the next syntax so that any Stacks table gets these a11y fixes for free! I currently have a warning in there telling devs in a non-prod environment to update their markup. Should that warning stay or should this transformation be considered a "feature?"

My vote is for the warning to stay. Tables (both the HTML element and our component) are convoluted enough that I'm in favor of almost any opportunity to unify the expected markup.

What do these changes NOT do?

This PR does not change/improve any screen reader announcements in terms of announcing new sorting or changing of table captions. For the moment, we'll continue to rely on an SR's support of aria-sort.

Resources

• W3 ARIA APG sortable table

• Deque sortable table

BTW, this is a killer enhancement. You da best 💛

To add to what @dancormier said I think this PR is a great step in the right direction to make our tables more accessible.
Thanks for initiating this @allejo! 💛

Here are some things I noticed browsing through the feature:

• This PR is associated to this branch from your forked repository but there is also a branch in this repo which has 3 more commits. I would recommend to close this PR and open a new one based on your branch in this repo so that we can get a correct diff.
• I would treat the sorted column highlighting feature as a completely separate task (keep this PR focused exclusively on a11y fixes). I am pretty sure we should consult our designer as well to get a sign off before being able to introduce that. Figma might need to be updated accordingly too.
• aria-sort rule is not working correctly - it looks like the attribute is changed in all the th elements at once. Probably this is because we update it to all elements without distinguishing which is the one that gets sorted (see here for details)
• Personally I am not a fan of introducing code to replace legacy markup dynamically. I would prefer to invest on a "codemod" instead that can be run at build time once. This will result in less complexity for our codebase and a leaner bundle size. The runtime warning for development bundles is a good idea but as far as my understanding goes we don't ship Stacks dev bundles for our consumers (yet).
• (the following is a bit out of scope but I want to mention it anyway): I feel very uncomfortable modifying algorithms without a solid test suite. I would love to prioritize introducing a unit/integration testing suite in Stacks before building on top of what we have. Here is some PoC work to clarify what I mean.

I believe as a next step someone from the core Stacks team should pick this item up and do a thorough review (potentially even pair with you).

How urgent is this a11y fix for you and your team @allejo?

Thanks again for initiating this! 💛

• This PR is associated to this branch from your forked repository but there is also a branch in this repo which has 3 more commits. I would recommend to close this PR and open a new one based on your branch in this repo so that we can get a correct diff.

Oh man, that's what I get for not noticing where Git is pushing my branches to 😅 I've deleted the branch that I pushed to the Stacks repo and will keep the one in my fork solely because I don't want the conversations going on here to be lost due (or behind an extra link/click)

• I would treat the sorted column highlighting feature as a completely separate task (keep this PR focused exclusively on a11y fixes). I am pretty sure we should consult our designer as well to get a sign off before being able to introduce that. Figma might need to be updated accordingly too.

I definitely agree with getting designers' eyes on this! I have no objections to moving that functionality out to a separate PR.

• aria-sort rule is not working correctly - it looks like the attribute is changed in all the th elements at once. Probably this is because we update it to all elements without distinguishing which is the one that gets sorted (see here for details)

Oops! Fixed 😄

• Personally I am not a fan of introducing code to replace legacy markup dynamically. I would prefer to invest on a "codemod" instead that can be run at build time once. This will result in less complexity for our codebase and a leaner bundle size. The runtime warning for development bundles is a good idea but as far as my understanding goes we don't ship Stacks dev bundles for our consumers (yet).

You have my attention about a "codemod" tool, what are you envisioning? I'm not fond of updating legacy markup at runtime each time but didn't have any better ideas that wouldn't require some more involved dev work. And you're right about not shipping dev bundles... Completely forgot about that; the runtime warning wouldn't even exist in the dist used in pubplat.

• (the following is a bit out of scope but I want to mention it anyway): I feel very uncomfortable modifying algorithms without a solid test suite. I would love to prioritize introducing a unit/integration testing suite in Stacks before building on top of what we have. Here is some PoC work to clarify what I mean.

💯 agree! I tried my best to not touch any of the sorting algorithm logic in this PR. But I would have been a lot more comfortable if I could have tests to lean on to check my work and adding new tests for button actions.

I believe as a next step someone from the core Stacks team should pick this item up and do a thorough review (potentially even pair with you).

I'm cool with this! 😄

How urgent is this a11y fix for you and your team @allejo?

Not urgent at the moment. I was working on redesigning some mod pages (that have already been shipped) that had tables and this a11y bug grabbed my attention so I picked it up.

@allejo I added your PR in our internal team backlog for visibility (you are tagged there too).
I would prefer if we could keep this PR just about the a11y fixes though.

It would be helpful if you could move the sorted column highlighting functionality/code in a different PR since it needs additional steps before we could pick it up for development (design review, figma update, etc...).

I additionally converted the PR as draft for now too until it's in a better state to be merged.

Thanks again for your work. 💛

I would prefer if we could keep this PR just about the a11y fixes though.

Of course! I have removed the column highlighting functionality from this PR and moved it over to #1165

I'm really excited by this change! We'll be wanting sortable tables in an internal project and having sorting be keyboard and screen-reader accessible would be a great thing.

Here's another link for reference: https://adrianroselli.com/2021/04/sortable-table-columns.html. It's marked out-of-scope for this PR but this link also presents various different ways to have sortability and sort change to be announced for screen readers. If we do enhance the feature with instructions/announcements here or in future, the HTML will need to provide localised/translated strings for these.

I think we should update the documentation in this PR to ensure devs write in the initial aria-sort attribute for the non-JS table examples where columns are already sorted. (It'd also apply to the JS examples, but those columns aren't pre-sorted in the examples)

Something like this for example:

--- a/docs/product/components/tables.html
+++ b/docs/product/components/tables.html
@@ -634,14 +634,14 @@ description: Tables are used to list all information from a data set. The base s
 <section class="stacks-section">
     {% header "h2", "Sortable tables" %}
     <p class="stacks-copy">To indicate that the user can sort a table by different columns, add the <code class="stacks-code">s-table__sortable</code> class to the table.</p>
-    <p class="stacks-copy">The <code class="stacks-code">&lt;th&gt;</code> cells should include arrows to indicate sortability or the currently applied sorting. In addition, the column that is currently sorted should be indicated with the <code class="stacks-code">is-sorted</code> class on its <code class="stacks-code">&lt;th&gt;</code>.</p>
+    <p class="stacks-copy">The <code class="stacks-code">&lt;th&gt;</code> cells should include arrows to indicate sortability or the currently applied sorting. In addition, the column that is currently sorted should be indicated with the <code class="stacks-code">is-sorted</code> class and the <code class="stacks-code">aria-sort</code> attribute with the <a href="https://www.w3.org/TR/wai-aria-1.1/#aria-sort">appropriate value</a> on its <code class="stacks-code">&lt;th&gt;</code>.</p>
     <div class="stacks-preview">
 {% highlight html %}
 <div class="s-table-container">
     <table class="s-table s-table__sortable">
         <thead>
             <tr>
-                <th scope="col" class="is-sorted">Listing @Svg.ArrowDownSm</th>
+                <th scope="col" class="is-sorted" aria-sort="descending">Listing @Svg.ArrowDownSm</th>
                 <th scope="col">Status @Svg.ArrowUpDownSm</th>
                 <th scope="col">Owner @Svg.ArrowUpDownSm</th>
                 <th scope="col" class="ta-right">Views @Svg.ArrowUpDownSm</th>
@@ -659,7 +659,7 @@ description: Tables are used to list all information from a data set. The base s
                 <table class="s-table s-table__sortable">
                     <thead>
                         <tr>
-                            <th scope="col" class="is-sorted">
+                            <th scope="col" class="is-sorted" aria-sort="descending">
                                 Listing
                                 {% icon "ArrowDownSm" %}
                             </th>

Screen readers can announce the initial state of the sorting even if interactive sorting isn't enabled for the table.

I've noticed that the styling doesn't quite match the previous appearance, especially noticeable in the font using the default <button> font instead of the bolded header font. The cursor is the regular arrow at the moment (instead of cursor) and it's also not possible to select and copy the header text any more:

I think it just needs some CSS tweaks to get it looking and behaving like before when the text was directly within the <th>. The s-btn__unset class has rules font: unset and user-select: auto that I think fix these. Plus I think cursor: pointer can be added to fix that cursor. Applying these new rules seem to fix things up:

(It might need additional tweaking for things I haven't noticed are not identical to before)

I'd love it if the headers had a focus shadow/outline like we see on other elements (right now it's default for button), but I have no idea how to get those showing up without being cropped by the boundaries of the <th> element.

Now that there's a test framework for Stacks, hopefully we can add in tests to verify no breaking changes here (e.g. test ensureHeadersAreClickable keeps working with future changes).

Great work on implementing this! 🥳 Let me know if there's anything I can help with re. testing or anything.